Skip to content

Conversation

@gagarc
Copy link

@gagarc gagarc commented Nov 6, 2024

Copied prte.c and made main into a library function while also renaming the copied filed to prtelauncher.c. Also removed prte.c from src/tools/prte/ while removing any references to prte.c in Makefile.am

@gagarc gagarc requested review from hppritcha and jsquyres November 6, 2024 02:33
@hppritcha
Copy link

It looks like we may need to update the pmix submodule to avoid the CI failure. @jsquyres I think this is happening because the CI is using HEAD of openpmix. someone (i wonder who) did a bunch of refactoring of pmix/prrte that removed PMIX_SESSION_PROVISION from the pmix header file(s).

@hppritcha
Copy link

i'm sorry - i may not be clear enough. We need to rebase the capstone-devel prrte on top of upstream. or we need to a patch to capstone-devel prrte to build against head of openpmix master.

@gagarc gagarc force-pushed the gerardo-refactor-mpirun branch from b247c15 to c2e29f0 Compare November 8, 2024 00:25
@jsquyres
Copy link

jsquyres commented Nov 8, 2024

Note that you got build errors across the board here in CI:

prted/pmix/pmix_server_queries.c:837:71: warning: incompatible pointer types passing 'void **' to parameter of type 'pmix_value_t **' (aka 'struct pmix_value **') [-Wincompatible-pointer-types]
                ret = PMIx_Get(&pproc, PMIX_MEM_ALLOC_KIND, &info, 1, (void**)&value);
                                                                      ^~~~~~~~~~~~~~
/Users/runner/work/_temp/pmixinstall/include/pmix.h:208:51: note: passing argument to parameter 'val' here
                                   pmix_value_t **val);
                                                  ^
prted/pmix/pmix_server_session.c:145:50: error: use of undeclared identifier 'PMIX_SESSION_PROVISION'
        } else if (PMIX_CHECK_KEY(&req->info[n], PMIX_SESSION_PROVISION) ||
                                                 ^
1 error generated.
make[2]: *** [prted/pmix/libprrte_la-pmix_server_session.lo] Error 1

You might want to try building before pushing up to the PR.

@hppritcha
Copy link

No the problem here doesn't have anything to do with the capstone work. it has to do with the fact that certain "changes" were made to the upstream openpmix that require us to either slurp up the corresponding changes in the upstream prrte master or figure out how to change the CI to use a working version of openpmix or else patch the capstone prrte devel branch so it can work with CI using head of upstream openpmix master or something. I believe btw, that open-mpi/ompi#12906 is failing owing to changes introduced in upstream prrte/openpmix that included the code changes that are now breaking CI for this project. So i'm reluctant to just patch both the Open MPI prrte and the capstone prrte with those changes in upstream prrte/openpmix.

@hppritcha
Copy link

One idea to try folks is to modify the .github/workflows yml files to build against a specific sha of openpmix.
See https://stackoverflow.com/questions/73879978/how-to-create-a-github-action-to-checkout-a-specific-commit-in-a-private-reposit

You would want to use the same openpmix sha as what we are currently using in Open MPI for main branch.
See https://github.com/open-mpi/ompi/tree/main/3rd-party. looks like a sha to try is 08e41ed

@rhc54
Copy link

rhc54 commented Nov 11, 2024

No the problem here doesn't have anything to do with the capstone work. it has to do with the fact that certain "changes" were made to the upstream openpmix that require us to either slurp up the corresponding changes in the upstream prrte master or figure out how to change the CI to use a working version of openpmix or else patch the capstone prrte devel branch so it can work with CI using head of upstream openpmix master or something.

Yes, PRRTE has always tracked with PMIx. However, I have gone through some effort to "protect" the code so that PRRTE will build with earlier versions. I'm afraid you just haven't kept up with the changes 🤷‍♂️ Current PRRTE head of master branch will build with older PMIx versions down to PMIx v4.2

add fetch depth 0

Till we figure out what got busted in upstream pmix/prrte combo.
See what's happening with

open-mpi/ompi#12906

Signed-off-by: Howard Pritchard <[email protected]>
@hppritcha
Copy link

okay looks like one more fixed needed for make distcheck

ERROR: files left after uninstall:
./lib/libprrte.so.4.0.0
make[1]: *** [Makefile:1026: distuninstallcheck] Error 1
make[1]: Leaving directory '/home/runner/work/prrte/prrte/prrte-gitclone/_build/sub'
make: *** [Makefile:973: distcheck] Error 1

…ry function and restructure any related components.

Removed prte.c and all references to it from the Makefile.am, and successfully extracted and refactored the main function from
prte.c into the library function prte_launcher.c. Also relocated prte_launcher.c to src/runtime to align with its role as a library
function rather than a tool in src/tools/prte. The directory src/tools/prte was also deleted and the build system was updated to reflect these
structural changes in PRRTE. This work is apart of the ongoing effort of integrating PRRTE functionality back into OMPI, avoiding the need to
fork/exec prterun.

Signed-off-by: Gerardo Garcia <[email protected]>
@gagarc gagarc force-pushed the gerardo-refactor-mpirun branch from a6ff81c to 5181ce5 Compare November 21, 2024 00:30
GitHub Actions: checkout specific pmix sha to use for CI
Copy link

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gagarc Go ahead and merge

@gagarc gagarc merged commit 2b0baf3 into capstone-devel Nov 25, 2024
11 checks passed
@gagarc gagarc deleted the gerardo-refactor-mpirun branch November 25, 2024 21:29
hppritcha added a commit to hppritcha/prrte that referenced this pull request May 5, 2025
…rrte.so

manual addition of much of the content of UofL work in

uofl-capstone-open-mpi#6

Signed-off-by: Howard Pritchard <[email protected]>
Signed-off-by: Gerardo Garcia <[email protected]>
hppritcha added a commit to hppritcha/prrte that referenced this pull request May 5, 2025
…rrte.so

manual addition of much of the content of UofL work in

uofl-capstone-open-mpi#6

Signed-off-by: Howard Pritchard <[email protected]>
Signed-off-by: Gerardo Garcia <[email protected]>
hppritcha pushed a commit to hppritcha/prrte that referenced this pull request Jun 2, 2025
Add configure CLI option to allow optionally prefixing PRTE binary and
library filenames.

prted: support --with-prte-binary-prefix

Add the prefix to the prted binary name that was passed in via
--with-prte-binary-prefix.

Add prefixing capability to prrte library and binaries

Updated Makefile.am files to include @PRTE_BINARY_PREFIX@ in library and
binary names where appropriate.
Verified successful build and functionality of prefixed binaries
This commit ensures compatiblity with prefixed binary naming while
maintaining the integrity of the build system and runtime.

Added and finalized ompi- prefix updates for PRRTE manpages.

Added ompi- prefix for all instances of prte commands within both the man/man1 and man/man5 directories, and also renamed necessary files to accommodate changes.

Updated and corrected ompi- prefix for prrte manpages

Finalized changes to prrte manpages with -ompi prefix

Changed configuration.rst file to fix warning that is being treated as an error

Also fixed docs/Makefile.am for appropriate file renames

Turned main function in prte.c into library function provided in libprrte.so

manual addition of much of the content of UofL work in

uofl-capstone-open-mpi#6

avoid defining PRTE_BINARY_PREFIX twice

makefile fixes for prrte binary prefixing

There were changes to PRRTE after UofL merged

uofl-capstone-open-mpi#9

into their fork of prrte.  These changes get it working again.

modify some makefiles that UofL somehow missed

Signed-off-by: Howard Pritchard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants